Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add PSA interruptible export public-key get num ops API #9820

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Add PSA interruptible export public-key get num ops API #9820

merged 1 commit into from
Dec 19, 2024

Conversation

waleed-elmelegy-arm
Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-arm commented Dec 2, 2024 •
edited
Loading

Description

fixes #9649

Add PSA interruptible export public-key get num ops API

TF-PSA-Crypto PR: Mbed-TLS/TF-PSA-Crypto#119

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog provided
  • development PR not required because:
  • framework PR not required
  • 3.6 PR not required because:
  • 2.28 PR not required because:
  • tests provided

@waleed-elmelegy-arm waleed-elmelegy-arm added DO-NOT-MERGE needs-ci Needs to pass CI tests component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Dec 2, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-export-pub-key-get-num-ops branch 2 times, most recently from d96914d to db56913 Compare December 3, 2024 16:03
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-preceding-pr Requires another PR to be merged first size-s Estimated task size: small (~2d) and removed DO-NOT-MERGE needs-ci Needs to pass CI tests labels Dec 3, 2024
@paul-elliott-arm paul-elliott-arm self-requested a review December 4, 2024 11:50
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-export-pub-key-get-num-ops branch from 53e7ad2 to ef5c8a9 Compare December 10, 2024 18:02
@paul-elliott-arm paul-elliott-arm added needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon labels Dec 11, 2024
paul-elliott-arm
paul-elliott-arm reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, looks good to me, obvious rebase incoming, so not approving yet.

@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-export-pub-key-get-num-ops branch from ef5c8a9 to 254b6f2 Compare December 12, 2024 12:43
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-reviewer This PR needs someone to pick it up for review and removed needs-preceding-pr Requires another PR to be merged first labels Dec 12, 2024
gilles-peskine-arm
gilles-peskine-arm reviewed Dec 12, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me for what is there, but either some coverage is missing or I missed some of the tests.

tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated

num_ops = psa_export_public_key_iop_get_num_ops(&export_key_operation);
TEST_EQUAL(num_ops, 0);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the sequence

/*first operation*/
setup()
complete() until SUCCESS
/*second operation using the same operation object*/
setup()
complete()
get_num_ops()

the call to get_num_ops should only get the op count from the second operation. Is this tested anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test for it.

tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated

num_ops = psa_export_public_key_iop_get_num_ops(&export_key_operation);
TEST_EQUAL(num_ops, 0);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the case of calling psa_export_public_key_iop_xxx on a public key object? Is this in scope for this pull request (I guess so since #9649 doesn't mention a limitation)?

Copy link
Contributor Author

@waleed-elmelegy-arm waleed-elmelegy-arm Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in scope. Added a test for it as well.

gilles-peskine-arm
gilles-peskine-arm previously approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

paul-elliott-arm
paul-elliott-arm previously approved these changes Dec 16, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paul-elliott-arm paul-elliott-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Dec 16, 2024
@ronald-cron-arm
Copy link
Contributor

Good news, I've been able to port this to TF-PSA-Crypto quite easily. I will share how I've done it.

@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-export-pub-key-get-num-ops branch from 1cdef01 to 5c3f36c Compare December 17, 2024 12:42
@waleed-elmelegy-arm waleed-elmelegy-arm mentioned this pull request Dec 17, 2024
Merged
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-export-pub-key-get-num-ops branch from 5c3f36c to 5f58453 Compare December 17, 2024 13:32
gilles-peskine-arm
gilles-peskine-arm previously approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed approved Design and code approved - may be waiting for CI or backports labels Dec 17, 2024
paul-elliott-arm
paul-elliott-arm previously approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paul-elliott-arm paul-elliott-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Dec 18, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-export-pub-key-get-num-ops branch from 5f58453 to e809ef4 Compare December 19, 2024 11:18
@waleed-elmelegy-arm
Copy link
Contributor Author

Updated the TF-PSA-Crypto submodule to point to merged commit.

gilles-peskine-arm
gilles-peskine-arm approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

yanesca
yanesca approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yanesca yanesca added this pull request to the merge queue Dec 19, 2024
Merged via the queue into Mbed-TLS:development with commit af8b3b5 Dec 19, 2024
6 checks passed
@waleed-elmelegy-arm waleed-elmelegy-arm mentioned this pull request Dec 19, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanesca yanesca yanesca approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@paul-elliott-arm paul-elliott-arm Awaiting requested review from paul-elliott-arm

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-psa PSA keystore/dispatch layer (storage, drivers, …) priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap pull requests (new board)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement interruptible ECC Export Public Key (iop tests)
5 participants
@waleed-elmelegy-arm @ronald-cron-arm @yanesca @gilles-peskine-arm @paul-elliott-arm